Skip to content

two-way conversions between Color-Vec4 and Color-[f32; 4] - #688

Merged
cart merged 5 commits into
bevyengine:masterfrom
tigregalis:master
Oct 19, 2020
Merged

two-way conversions between Color-Vec4 and Color-[f32; 4]#688
cart merged 5 commits into
bevyengine:masterfrom
tigregalis:master

Conversation

@tigregalis

@tigregalis tigregalis commented Oct 16, 2020

Copy link
Copy Markdown
Contributor

Currently you can convert one way from Vec4->Color, and one way from Color->[f32; 4].

Added From trait implementations for Color to allow two-way conversions between Color<->Vec4 and Color<->[f32; 4].

As a side note, replaced impl Into<[f32; 4]> for Color with impl From<Color> for [f32; 4] which is better practice according to Trait std::convert::From.

This is practically my first PR ever - let me know if there's something I've missed / should have done.

@cart

cart commented Oct 16, 2020

Copy link
Copy Markdown
Member

Congrats on the first-ish pr! This looks good to me and I think its mergeable as-is, but I also think we have an opportunity here to resolve a mismatch in the Color into/from conversions that we introduced in #616. Looks like we forgot to change the Into<Color> impl in that pr.

Now that the "public" interface Color presents is Srgb, i think it makes sense to make the conversions do to/from conversions to Srgb.

For anything converting to Color, can we use the Color::rgba() constructor instead of directly creating the struct like Color { r, g, b, a }. This will ensure that we do srgb conversions.

Likewise for anything extracting values from color (ex: [f32; 4]), can we use color.r(), color.g(), color.b(), and color.a()?

@tigregalis
into Color use rgba constructor, from Color use getters.
f616950 
added `impl From<[f32; 4]> for ColorSource` for consistency.
@tigregalis

tigregalis commented Oct 17, 2020

Copy link
Copy Markdown
Contributor Author

Thanks. I've learnt something new.

I've made a new commit - is this what you had in mind?

@tigregalis

tigregalis commented Oct 17, 2020

Copy link
Copy Markdown
Contributor Author

Perhaps I should make this comment on #616.

impl Mul and impl MulAssign

Should these be changed as well to reflect the public nonlinear API?

Without it:

let starting_vec4 = Vec4::new(0.4, 0.5, 0.6, 1.0);
let starting_color = Color::from(starting_vec4);
dbg!(starting_color);
assert_eq!(
    starting_vec4,
    Vec4::from(starting_color),
); // passes
let transformation = Vec4::new(0.5, 0.5, 0.5, 1.0);
assert_eq!(
    starting_color * transformation,
    Color::from(starting_vec4 * transformation),
); // fails

outputs:

[src\main.rs:45] starting_color = Color {
    red: 0.13286833,
    green: 0.21404114,
    blue: 0.31854683,
    alpha: 1.0,
}
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `Color { red: 0.06643417, green: 0.10702057, blue: 0.15927342, alpha: 1.0 }`,
 right: `Color { red: 0.033104762, green: 0.05087609, blue: 0.07323897, alpha: 1.0 }`', src\main.rs:51:1

What should the behaviour be?

impl Mul<f32> for Color

Is there value in impl Mul<f32> for Color, at least with its current behaviour?

// crates/bevy_render/src/color.rs
impl Mul<f32> for Color {
    type Output = Color;

    fn mul(self, rhs: f32) -> Self::Output {
        Color {
            red: self.red * rhs,
            green: self.green * rhs,
            blue: self.blue * rhs,
            alpha: self.alpha * rhs,
        }
    }
}

I'm not certain of the usefulness of multiplying the alpha.

The same can be achieved with color * Vec3::splat(some_f32) or color * Vec4::splat(some_f32) and it is more explicit.

Separate types

I'm beginning to think these could be separate types that can be converted between each other, or perhaps variants of an enum.

Outline would be something like:

// crates\bevy_render\src\color.rs
pub struct Color;

impl Color {
    pub const fn rgba(r: f32, g: f32, b: f32, a: f32) -> Self { /* ... */ }
    pub fn to_linear(self) -> ColorLinear { /* ... */ }
}

pub struct ColorLinear;

impl ColorLinear {
    pub const fn rgba(r: f32, g: f32, b: f32, a: f32) -> Self { /* ... */ }
    pub fn to_nonlinear(self) -> Color { /* ... */ }
}

impl From<ColorLinear> for Color {
    fn from(color_linear: ColorLinear) -> Self {
        ColorLinear::to_nonlinear(color_linear)
    }
}

impl From<Color> for ColorLinear {
    fn from(color_nonlinear: Color) -> Self {
        Color::to_linear(color_nonlinear)
    }
}

pub enum ColorSource {
    ColorLinear(ColorLinear),
    Color(Color),
    Texture(Handle<Texture>),
}

impl From<Color> for ColorSource { /* ... */ }
impl From<ColorLinear> for ColorSource { /* ... */ }

impl From<Vec4> for Color { /* ... */ }
impl From<Color> for Vec4 { /* ... */ }
impl From<Vec4> for ColorLinear { /* ... */ }
impl From<ColorLinear> for Vec4 { /* ... */ }
// etc.

// crates\bevy_pbr\src\material.rs
impl From<Color> for StandardMaterial { /* ... */ }
impl From<ColorLinear> for StandardMaterial { /* ... */ }

// crates\bevy_sprite\src\color_material.rs
impl From<Color> for ColorMaterial { /* ... */ }
impl From<ColorLinear> for ColorMaterial { /* ... */ }

// everywhere else, Find-Replace /\bColor\b/g for "ColorLinear"

@memoryruins memoryruins added the C-Code-Quality A section of code that is hard to understand or change label Oct 17, 2020
@cart

cart commented Oct 18, 2020

Copy link
Copy Markdown
Member

Ultimately I think separate types are the right way to go, but that will require slightly smarter gpu data conversions, so I'd rather handle that later.

For now, I do think it makes sense to fix the math ops. I think it makes sense to adjust them to use nonlinear srgb everywhere:

// crates/bevy_render/src/color.rs
impl Mul<f32> for Color {
    type Output = Color;

    fn mul(self, rhs: f32) -> Self::Output {
        Color::rgba(self.r() * rhs.r(), ...);
    }
}

@tigregalis

tigregalis commented Oct 19, 2020

Copy link
Copy Markdown
Contributor Author

Ok, I've made those changes.

@cart
cart merged commit 03bc5d7 into bevyengine:master Oct 19, 2020
@tigregalis tigregalis mentioned this pull request Jan 18, 2021
Closed
@tigregalis tigregalis mentioned this pull request Feb 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

C-Code-Quality A section of code that is hard to understand or change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tigregalis @cart @memoryruins